Fix the implementation of BlowFish to solve bugs concerning NUL bytes truncating strings by LamentXU123 · Pull Request #21675 · php/php-src

LamentXU123 · 2026-04-08T13:39:24Z

Now, since including \0 in password_hash is not allowed in PASSWORD_BCRYPT. It is reasonable to add checkers that, makes input that includes \0 in password_verify directly fails.

This PR align password_verify() with password_hash() for bcrypt by rejecting passwords containing NUL bytes before verification.

Because bcrypt treats passwords as NUL-terminated C strings, this avoids ambiguous verification behavior for inputs such as "foo\0bar". A regression test is included.

Fix #21673

To be short: password encrypted by BYCRYPT should never includes \0. So directly rejecting it is always correct. It also avoids ambiguous verification where something like this happens:

<?php
$hash = password_hash("secret", PASSWORD_BCRYPT);
var_dump(password_verify("secret" . chr(0) . "suffix", $hash)); //True, WTF?
?>

ext/standard/password.c

LamentXU123 · 2026-04-09T07:40:46Z

ext/standard/password.c

 	zend_long cost = PHP_PASSWORD_BCRYPT_COST;

-	if (memchr(ZSTR_VAL(password), '\0', ZSTR_LEN(password))) {
+	if (zend_str_has_nul_byte(password)) {


nit: I also refactor this by the way.

alecpl · 2026-04-09T12:00:20Z

Shouldn't that be rather done in php_password_bcrypt_verify()?

LamentXU123 · 2026-04-09T12:03:18Z

Shouldn't that be rather done in php_password_bcrypt_verify()?

It is before, see the initial fix in 6f14bee

Actually I think that'd be better. Anyway, lets wait for code owners.

Girgias · 2026-04-09T12:34:44Z

Can we not fix the blowfish implementation? It seems like it requires providing the length of the string all the way down to BF_set_key. Or does BlowFish specifically not support null bytes?

LamentXU123 · 2026-04-09T12:51:51Z

Does BlowFish specifically not support null bytes?

Blowfish do support null bytes, but in php implementation it specifically doesn't. Any password with NUL will fail when hashing by using password_hash in bcrypt mode.

bugs with NUL bytes is inevitable since PHP is based on C string. But in this case, I think directly rejecting all inputs containing NUL bytes will be safer and always correct.

Girgias · 2026-04-10T10:15:39Z

Does BlowFish specifically not support null bytes?

Blowfish do support null bytes, but in php implementation it specifically doesn't. Any password with NUL will fail when hashing by using password_hash in bcrypt mode.

bugs with NUL bytes is inevitable since PHP is based on C string. But in this case, I think directly rejecting all inputs containing NUL bytes will be safer and always correct.

Sure, but we "own" the implementation of BF, so why not actually fix the implementation? I'm happy to reject them as a bug fix that is backported, but in the long run we should fix the implementation.

LamentXU123 · 2026-04-10T10:47:04Z

Does BlowFish specifically not support null bytes?

Blowfish do support null bytes, but in php implementation it specifically doesn't. Any password with NUL will fail when hashing by using password_hash in bcrypt mode.
bugs with NUL bytes is inevitable since PHP is based on C string. But in this case, I think directly rejecting all inputs containing NUL bytes will be safer and always correct.

Sure, but we "own" the implementation of BF, so why not actually fix the implementation? I'm happy to reject them as a bug fix that is backported, but in the long run we should fix the implementation.

In the long run we should. But for now, password_hash does not accept password containing NUL, so fixing the implementation would create a BC break on this which requires a RFC. I would be happy to write such RFC in the future but I think this bug could be fix before we get it passed.

Girgias · 2026-04-10T10:55:33Z

I don't think fixing the implementation to be correct is a BC break.
Your bug fix is a BC break by this logic...

LamentXU123 · 2026-04-10T11:00:25Z

I don't think fixing the implementation to be correct is a BC break.

That'd be good. I will work on this hours later.

I assume that the best practice here would be passing the correct length throughout the implementation so the strings will not be cut by NUL

Girgias · 2026-04-10T11:02:10Z

I don't think fixing the implementation to be correct is a BC break.

That'd be good. I will work on this hours later.

I assume that the best practice here would be passing the correct length throughout the implementation so the strings will not be cut by NUL

Correct :)

ext/standard/crypt.c

ext/standard/crypt_blowfish.c

ext/standard/tests/strings/crypt_blowfish_null.phpt

Girgias · 2026-04-13T10:32:51Z

ext/standard/crypt.c

 		Z_PARAM_STRING(str, str_len)
 		Z_PARAM_STRING(salt_in, salt_in_len)


We may want these to be Z_PARAM_PATH if we use the system crypt. Or Maybe it should indeed always be PATH so that the behaviour is consistent with the system API...

We may want these to be Z_PARAM_PATH

I implement the bcrypt() to reject NUL byte, and make password_* not to.

Not sure about this change.

ext/standard/crypt.c

Co-authored-by: Gina Peter Banyard <girgias@php.net>

Girgias

Yeah, not sure how I feel about this either. Maybe @TimWolla has an opinion?

ext/standard/crypt.c

TimWolla · 2026-04-13T15:01:24Z

ext/standard/crypt_blowfish.c

This file is a vendored library and must not be touched.

TimWolla · 2026-04-13T15:06:34Z

Or does BlowFish specifically not support null bytes?

It's not BlowFish, it's Bcrypt. And no, most Bcrypt implementation don't support NULs.

ndossche · 2026-04-13T19:10:05Z

The idiotic thing about how PHP implements password hashing is that it doesn't always use the builtin bcrypt implementation: it can defer to crypt_r which doesn't support NUL bytes. So if you want to fix this you'll need to ditch that other code path but that runs into political issues about "bundling too much code".

bukka · 2026-04-13T21:22:23Z

I guess the idea was that crypt implementation would be more secure but not sure if there's much value in it.

bukka · 2026-04-13T21:28:07Z

I'm also not sure if there's much value in allowing nul bytes in password. I think the initial idea of not allowing in the verify was more reasonable. That was sort of the plan when we were handling GHSA-h746-cjrr-wfmr - prohibit the hashing as low security issue and add warning for verify in master.

LamentXU123 · 2026-04-14T02:41:03Z

I don't have strong opinions on both solutions, but I would revert back to the initial fix I guess if we are following GHSA-h746-cjrr-wfmr

plus: Is this issue worth a separate security entry? I mean that password_verify doesn't check if the input has NUL bytes could have low impact on some login scenarios.

…ul-2

TimWolla · 2026-04-14T14:55:07Z

I mean that password_verify doesn't check if the input has NUL bytes could have low impact on some login scenarios.

Please explain the impact you're seeing. I'm seeing none and I don't think this change is necessary either. For password_hash it's useful to not accidentally generate “weak hashes”, but for password_verify rejecting NULs will just break some working logic without any upside.

initial fix

6f14bee

LamentXU123 requested a review from bukka as a code owner April 8, 2026 13:39

github-actions bot added the Extension: standard label Apr 8, 2026

Update NEWS

512fcdb

devnexen reviewed Apr 8, 2026

View reviewed changes

ext/standard/password.c Outdated Show resolved Hide resolved

Update password.c

08d7fcc

LamentXU123 commented Apr 9, 2026

View reviewed changes

LamentXU123 mentioned this pull request Apr 9, 2026

Use zend_str_has_nul_byte instead of memchr #21680

Merged

initial implementation

39af57c

github-actions bot added the ABI break label Apr 10, 2026

LamentXU123 changed the title ~~Fix GH-21673: password_verify() failed to verify bcrypt passwords containing null bytes~~ Fix the implementation of BlowFish to solve bugs concerning NUL bytes truncating strings Apr 10, 2026

Girgias reviewed Apr 10, 2026

View reviewed changes

ext/standard/crypt.c Show resolved Hide resolved

ext/standard/crypt_blowfish.c Outdated Show resolved Hide resolved

ext/standard/tests/strings/crypt_blowfish_null.phpt Outdated Show resolved Hide resolved

correct the styles

631983e

Girgias requested changes Apr 13, 2026

View reviewed changes

LamentXU123 and others added 2 commits April 13, 2026 18:40

Apply suggestion from @Girgias

73f3a1c

Co-authored-by: Gina Peter Banyard <girgias@php.net>

patch

c5e9672

Girgias reviewed Apr 13, 2026

View reviewed changes

ext/standard/crypt.c Outdated Show resolved Hide resolved

TimWolla self-requested a review April 13, 2026 14:59

TimWolla requested changes Apr 13, 2026

View reviewed changes

ext/standard/crypt_blowfish.c

Copy link
Copy Markdown

Member

TimWolla Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is a vendored library and must not be touched.

LamentXU123 added 3 commits April 14, 2026 11:05

revert to initial fix

523c32c

revert to initial fix

9e63761

Merge branch 'vul-2' of https://github.com/LamentXU123/php-src into v…

049ba32

…ul-2

		Z_PARAM_STRING(str, str_len)
		Z_PARAM_STRING(salt_in, salt_in_len)

Conversation

LamentXU123 commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

LamentXU123 Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

alecpl commented Apr 9, 2026

Uh oh!

LamentXU123 commented Apr 9, 2026

Uh oh!

Girgias commented Apr 9, 2026

Uh oh!

LamentXU123 commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Girgias commented Apr 10, 2026

Uh oh!

LamentXU123 commented Apr 10, 2026

Uh oh!

Girgias commented Apr 10, 2026

Uh oh!

LamentXU123 commented Apr 10, 2026

Uh oh!

Girgias commented Apr 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Girgias Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

LamentXU123 Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Girgias left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TimWolla Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

TimWolla commented Apr 13, 2026

Uh oh!

ndossche commented Apr 13, 2026

Uh oh!

bukka commented Apr 13, 2026

Uh oh!

bukka commented Apr 13, 2026

Uh oh!

LamentXU123 commented Apr 14, 2026

Uh oh!

TimWolla commented Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

LamentXU123 commented Apr 8, 2026 •

edited

Loading

LamentXU123 commented Apr 9, 2026 •

edited

Loading